Autoroll from main to 27.lts - #11578
Conversation
Refer to original PR: #11550 Add support for a page alignment flag in DecoderBufferAllocator to control OS block allocation alignment. This allows for more granular memory management when using the configurable decommit strategy. Implement a lockless optimization during suspend by checking an atomic flag before acquiring the allocator mutex. This prevents unnecessary blocking of the UI thread when decommit on suspend is not enabled. Additionally, simplify pointer arithmetic in StarboardMemoryAllocator using uint8_t pointers and update H5vcc settings to expose the new alignment configuration bit. Bug: 454441375 (cherry picked from commit 8ac74bd)
🤖 Gemini Suggested Commit Message💡 Pro Tips for a Better Commit Message:
|
There was a problem hiding this comment.
Code Review
This pull request removes the MediaResourceTracker utility and the kWaitForMediaResourcesOnShutdown feature across Android Starboard media components, and introduces a page alignment configuration (allocate_with_page_alignment) to the decoder buffer allocator strategy and StarboardMemoryAllocator. A critical bug was identified in JavaSwitches.java where generating separate --enable-features and --disable-features arguments for each switch will cause only the last occurrence to be preserved by base::CommandLine, silently ignoring the others. It is recommended to revert this change and continue joining the features with commas into single arguments.
| if (javaSwitches.containsKey(JavaSwitches.USE_IPV4_FOR_DNS)) { | ||
| enabledFeatures.add("UseIPv4ForDNS"); | ||
| extraCommandLineArgs.add("--enable-features=UseIPv4ForDNS"); | ||
| } | ||
|
|
||
| if (javaSwitches.containsKey(JavaSwitches.LOCAL_STORAGE_DELETE_LOCK_FILE)) { | ||
| enabledFeatures.add("LocalStorageDeleteLockFile"); | ||
| extraCommandLineArgs.add("--enable-features=LocalStorageDeleteLockFile"); | ||
| } |
There was a problem hiding this comment.
Critical Bug: Multiple --enable-features and --disable-features arguments are overwritten
In Chromium and Cobalt, base::CommandLine parses command-line arguments into a map where each switch key can only have a single value. If a switch like --enable-features or --disable-features is specified multiple times on the command line, only the last occurrence's value is preserved, and all previous ones are overwritten and ignored.
By changing the code to append separate --enable-features=... arguments for each feature instead of joining them with a comma into a single --enable-features argument, only the last enabled feature in the list will actually be enabled. All other features will be silently ignored.
Suggested Fix
Revert this change and continue to collect all enabled/disabled features using a StringJoiner (or List<String>) and append a single --enable-features=... and --disable-features=... argument to extraCommandLineArgs at the end of the method, for example:
public static List<String> getExtraCommandLineArgs(Map<String, String> javaSwitches) {
List<String> extraCommandLineArgs = new ArrayList<>();
StringJoiner jsFlags = new StringJoiner(";");
StringJoiner enabledFeatures = new StringJoiner(",");
StringJoiner disabledFeatures = new StringJoiner(",");
if (javaSwitches.containsKey(JavaSwitches.USE_IPV4_FOR_DNS)) {
enabledFeatures.add("UseIPv4ForDNS");
}
// ... collect other features ...
if (enabledFeatures.length() > 0) {
extraCommandLineArgs.add("--enable-features=" + enabledFeatures.toString());
}
if (disabledFeatures.length() > 0) {
extraCommandLineArgs.add("--disable-features=" + disabledFeatures.toString());
}
return extraCommandLineArgs;
}
Automated cherry-pick roll to 27.lts.
Original pull requests: